Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement http3.Server.ServeListener #3349

Merged
merged 10 commits into from
Mar 21, 2022
Merged

Implement http3.Server.ServeListener #3349

merged 10 commits into from
Mar 21, 2022

Conversation

renbou
Copy link
Contributor

@renbou renbou commented Mar 17, 2022

Fixes #3347

Added a http3.Server.ServeListener (http3.Server.Serve is already taken and serves net.PacketConn) method which is can be used to serve content on an already created quic.EarlyListener which works like http.Server.Serve in net/http.

Originally, however, it was planned for these listeners not to be closed using http3.Server.Close so that http3.Servers can be switched between without closing/starting another listener. It turned out that this is easier implemented externally (for example Caddy uses "fake close" wrappers around listeners), since there isn't an easy way to interrupt the Accept running in http3.Server.serveImpl which was run using an quic.EarlyListener without closing the listener.

Note that net/http's Server.Close also closes the listener passed to Server.Serve, so the behavior is even more similar this way.

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This already looks very good to me. Thank you for working on this!

Two things:

  1. See my comment below regarding avoiding the massive callback?
  2. Can we have tests? ;) We definitely need a unit test in this package. Ideally, we'll also have an integration test in the integrationtests package, where we swap out the server (basically the thing that Caddy will do).

http3/server.go Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #3349 (1194748) into master (9c8cadb) will increase coverage by 0.02%.
The diff coverage is 90.70%.

@@            Coverage Diff             @@
##           master    #3349      +/-   ##
==========================================
+ Coverage   85.50%   85.52%   +0.02%     
==========================================
  Files         135      135              
  Lines        9830     9837       +7     
==========================================
+ Hits         8405     8413       +8     
+ Misses       1047     1046       -1     
  Partials      378      378              
Impacted Files Coverage Δ
http3/server.go 70.18% <90.70%> (+0.78%) ⬆️
internal/utils/rand.go 75.00% <0.00%> (+12.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c8cadb...1194748. Read the comment docs.

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far, but there seems to be a race condition in one of the tests.

@renbou
Copy link
Contributor Author

renbou commented Mar 21, 2022

The tests are fixed now, doesn't seem like the CircleCI fail is related to my changes

@marten-seemann marten-seemann merged commit b7e93b5 into quic-go:master Mar 21, 2022
@marten-seemann
Copy link
Member

Thank you @renbou!

nmldiegues pushed a commit to chungthuang/quic-go that referenced this pull request Jun 6, 2022
* feat(http3): implement serving from quic.Listener

ServeListener method added to http3.Server allowing serving from an existing listener
ConfigureTLSConfig function added to http3 which should be used to create listeners meant for serving http3.

* docs(http3): add note about using ConfigureTLSConfig to ServeListener

* fix(http3): stop serving non-created listeners after Server.Close

* refactor(http3): return ErrServerClosed once server closes instead of context.Canceled

* feat(http3): close listeners from ServeListener as well

* fix(http3): fix logger not being setup during ServeListener

* test(http3): add unit tests for serving listeners

* test(http3): add tests for ConfigureTLSConfig

* test(http3): added server hotswapping integration test

* fix: race condition in listener tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

introduce a http3.Server.Serve
2 participants